Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RAC] Build a consolidated implementation in Rule Registry (Draft) #101453

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Jun 6, 2021

Ticket: #101016

Summary

This is a draft of the consolidated implementation of index management for RAC indices (see #98912).

TODO:

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@banderror banderror requested review from marshallmain and xcrzx June 6, 2021 15:20
@banderror banderror self-assigned this Jun 6, 2021
@banderror banderror added the Theme: rac label obsolete label Jun 6, 2021
@banderror banderror force-pushed the consolidate-rule-registry-implementations branch 2 times, most recently from d211834 to 739f965 Compare June 7, 2021 10:01
@xcrzx xcrzx force-pushed the consolidate-rule-registry-implementations branch from 739f965 to 6733d09 Compare June 14, 2021 10:00
@xcrzx xcrzx added the Team:Detections and Resp Security Detection Response Team label Jun 14, 2021
@xcrzx xcrzx force-pushed the consolidate-rule-registry-implementations branch 2 times, most recently from e087a1a to ada916f Compare June 14, 2021 16:53
} catch (e) {
this.logger.error(
`error writing bulk events: "${e.message}"; docs: ${JSON.stringify(bulkBody)}`
);
return undefined;
Copy link
Contributor

@xcrzx xcrzx Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll probably need to return the error here or rethrow it. So we can properly handle errors in the caller methods.

@@ -60,11 +63,16 @@ export class IndexWriter {
}
}

public async indexManyNow(docs: Document[]): Promise<estypes.BulkResponse | undefined> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about renaming the methods of this class to make things a little bit more clear?
indexManyNow -> indexMany
indexOne -> enqueueOne
indexMany -> qnqueueMany

@banderror banderror force-pushed the consolidate-rule-registry-implementations branch from 897b4a7 to fa04b8d Compare June 30, 2021 15:17
@kibanamachine
Copy link
Contributor

kibanamachine commented Jun 30, 2021

💔 Build Failed

Failed CI Steps

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @banderror

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Detections and Resp Security Detection Response Team Theme: rac label obsolete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants